Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[fix][cli] Fix start cli error #2024

Merged
merged 6 commits into from
Nov 22, 2022
Merged

[fix][cli] Fix start cli error #2024

merged 6 commits into from
Nov 22, 2022

Conversation

coderzc
Copy link
Member

@coderzc coderzc commented Nov 20, 2022 •
edited
Loading

Motivation

Run bin/hugegraph start error

Starting HugeGraphServer...
bin/hugegraph: line 68: /start-hugegraph.sh: No such file or directory
Failed to start service

Modifications

Add check for Variables

@coderzc coderzc changed the title [Fix] Fix start shell error [fix][cli] Fix start shell error Nov 20, 2022
@coderzc coderzc changed the title [fix][cli] Fix start shell error [fix][cli] Fix start cli error Nov 20, 2022
@coderzc coderzc added this to the 1.0.0 milestone Nov 20, 2022
@coderzc coderzc added the bug Something isn't working label Nov 20, 2022
zyxxoo
zyxxoo previously approved these changes Nov 20, 2022
View reviewed changes
@codecov
Copy link

codecov bot commented Nov 20, 2022 •
edited
Loading

Codecov Report

Merging #2024 (3c88b2d) into master (4c232a6) will decrease coverage by 5.50%.
The diff coverage is n/a.

@@             Coverage Diff              @@
##             master    #2024      +/-   ##
============================================
- Coverage     64.38%   58.88%   -5.51%     
  Complexity      978      978              
============================================
  Files           482      482              
  Lines         41463    41463              
  Branches       5890     5890              
============================================
- Hits          26698    24414    -2284     
- Misses        12122    14616    +2494     
+ Partials       2643     2433     -210
Impacted Files Coverage Δ
...hugegraph/backend/store/raft/RaftStoreClosure.java 0.00% <0.00%> (-100.00%) ⬇️
...egraph/backend/serializer/BinaryEntryIterator.java 0.00% <0.00%> (-87.04%) ⬇️
...ugegraph/backend/store/rocksdb/RocksDBMetrics.java 0.00% <0.00%> (-85.87%) ⬇️
...gegraph/backend/store/rocksdb/RocksDBFeatures.java 4.16% <0.00%> (-83.34%) ⬇️
...baidu/hugegraph/backend/store/raft/RaftResult.java 0.00% <0.00%> (-81.25%) ⬇️
...aidu/hugegraph/backend/store/raft/RaftContext.java 0.00% <0.00%> (-80.96%) ⬇️
...hugegraph/backend/store/raft/RaftBackendStore.java 0.00% <0.00%> (-75.27%) ⬇️
...hugegraph/backend/store/raft/rpc/RpcForwarder.java 0.00% <0.00%> (-69.50%) ⬇️
...ugegraph/backend/store/raft/StoreStateMachine.java 0.00% <0.00%> (-65.77%) ⬇️
.../backend/store/raft/rpc/StoreCommandProcessor.java 0.00% <0.00%> (-65.22%) ⬇️
... and 60 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

imbajin
imbajin reviewed Nov 20, 2022
View reviewed changes
hugegraph-dist/src/assembly/static/bin/hugegraph Outdated
Comment on lines 39 to 40
# Variables
INSTALL_DIR=
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

change the comment, it requires user to set a fixed abs path manually (should't use a relative path)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right. and I think should add check for Variables

@coderzc coderzc removed the bug Something isn't working label Nov 20, 2022
javeme
javeme reviewed Nov 20, 2022
View reviewed changes
hugegraph-dist/src/assembly/static/bin/hugegraph
@@ -37,6 +37,7 @@
### END INIT INFO

# Variables
# it requires user to set a fixed abs path manually
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we can try ${INSTALL_DIR:?"Please setting variables 'INSTALL_DIR'"}
for more details: https://stackoverflow.com/a/16753536

Copy link
Member

@imbajin imbajin Nov 29, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://stackoverflow.com/a/16753536

@javeme seems a little strange, could it use directly or test it in local evn? (or should in a if condition?)

image

and it alert:
image

failed with all cases in my ubuntu21

@coderzc coderzc requested a review from javeme November 21, 2022 09:08
imbajin
imbajin reviewed Nov 21, 2022
View reviewed changes
hugegraph-dist/src/assembly/static/bin/hugegraph Outdated
@@ -41,13 +41,15 @@
INSTALL_DIR=
SERVER_PORT=

${INSTALL_DIR:?"Please setting variables 'INSTALL_DIR'"}
Copy link
Member

@imbajin imbajin Nov 21, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😺, nice

typo `setting --> set``

javeme
javeme previously approved these changes Nov 21, 2022
View reviewed changes
imbajin
imbajin approved these changes Nov 21, 2022
View reviewed changes
Copy link
Member

@imbajin imbajin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll enhance this script later

@javeme javeme merged commit 7acd496 into master Nov 22, 2022
@javeme javeme deleted the fix_start_shell branch November 22, 2022 12:56
imbajin pushed a commit to z7658329/hugegraph that referenced this pull request Nov 23, 2022
@coderzc @imbajin
[fix][cli] Fix start cli error (apache#2024)
d422616
@imbajin imbajin mentioned this pull request Nov 29, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@imbajin imbajin imbajin approved these changes

@javeme javeme javeme approved these changes

@zyxxoo zyxxoo zyxxoo left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
1.0.0
Development

Successfully merging this pull request may close these issues.
4 participants
@coderzc @javeme @zyxxoo @imbajin